-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add ros2 param #95
add ros2 param #95
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review. I used get and list so far and they seem to work fine.
ros2param/ros2param/verb/list.py
Outdated
future = client.call_async(request) | ||
futures[node_name] = future | ||
else: | ||
print(node_name, 'not ready') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are a few debug print statements. Use the node's logger for debug messages instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the debug output in 55a5948.
Tested this along ros2/rclcpp#489 and it works great 👍 I pushed a few nitpicks, feel free to revert them if they're not appropriate. A few questions that come to mind:
|
Additional feature can be added anytime. You could add it to this PR right now or keep it for a future PR. I just wanted to get a first version of this verb in to be able to use / test the parameters more easily. |
Follow up is fine by me 👍 it's great to have a first version of this verb out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for pounding on this 👍
Retroactive summary for this feature: add ros2 paramPurpose / Use Cases (Why implement this feature?)This was implemented to provide a way to set and get parameters of ROS nodes which have parameters and a parameter service (allowing parameters to be set/checked via ROS). Design (How does it work?)It provides a new "command" to the Inputs / Outputs / API (How to use it?)The usage of the tool is documented in the help text with There is a "public" API which implements the actions the tool can take (used internally by the tool), see:
|
list
outputs the names of all parameters, either for all nodes or a node passed as an argumentget
outputs the value of a specific parameter, the type information can be suppressedset
sets a parameter, the type is being determined by guessing the primitive types, no array types atmdelete
removed a specific parameter (at least it should but it seems that the service doesn't work for that use case).